Added code to demonstrate use of new I2C timeout feature #356

paynterf · 2020-07-11T03:38:44Z

Revised to demonstrate how to use the new I2C bus timeout feature

matthijskooijman

Looks good to me, I left a minor comment inline.

I think it would additionally be good to just define a timeout in all master examples, so that anyone who starts by copying an example will be safe from lockups. I would think just the timeout (with autoreset) is sufficient, no need to handle the timeout flags I think. As for the timeout, maybe 3ms is fine? Or use the higher value used by SMBus (can't remember the exact value just now)?

matthijskooijman · 2020-07-16T12:24:13Z

libraries/Wire/examples/i2c_scanner/i2c_scanner.ino

+
+  Wire.setWireTimeout(3000, true); //timeout value in uSec, true to reset I2C bus on timeout
+  wireTimeoutCount = 0;
+  Wire.clearWireTimeoutFlag();


Why not put this directly after the Wire.begin()? Seems more clear? Also, is clearing the timeout flag really needed here? We can rely on it being cleared on startup, I think? Same for initializing the count to 0, why not just do that in the variable declaration?

Thanks for the comment; I moved the anti-lockup code to just under 'Wire.begin()' as you suggested. I wanted to put the timeout flag in to expose its existence to a new user; otherwise they might never know the timeout flag existed at all or how to query it.

I could initialize the variable in the declaration, but I have learned that assuming a variable has been initialized to zero 'somewhere else' can lead to confusing/incorrect behavior. I like making things obvious and explicit, even at the cost of an extra line or two of code. YMMV ;-).

I wanted to put the timeout flag in to expose its existence to a new user; otherwise they might never know the timeout flag existed at all or how to query it.

Yeah, that's fine (and I think this might be a good example for it), I was just suggesting not doing the timeout flag handling in all examples (OTOH, maybe it should be).

I like making things obvious and explicit, even at the cost of an extra line or two of code. YMMV ;-).

It does (on this particular thing, I do like explicit in general, but also concise), but it's ok :-)

What about (not) calling clearWireTimeoutFlag() on startup? You've now added a comment that seems less than helpful?

Good catch! I removed the call to clearWireTimeoutFlag() as it is explicitly cleared in Wire.setWireTimeout()

…in 'SetTimeout()'

matthijskooijman

LGTM now!

bperrybap · 2020-09-07T18:41:26Z

I'm not sure where the put the reminder about needed a macro in the Wire library so sketches and libraries can detect the existence of the timeout API functions like setWireTimeout() but a macro needs to exist so that code can tell if the timeout api functions exist.
Further comments here: #107 (comment)

matthijskooijman · 2020-09-17T19:29:41Z

I added the macro that @bperrybap suggested in #362. Would it be good to explicitly check for this macro in these examples too? One one hand it is pointless, since these examples are shipped with a core that has the feature, but on the other hand it does result in more portable sketches and maybe raises awareness that not all cores have this timeout feature.

bperrybap · 2020-09-17T20:14:07Z

I think clear documentation is the most important thing.
Not sure where things like these WIRE_HAS_* macros would really get documented.
I guess that it belongs on the Wire reference pages:
https://www.arduino.cc/en/reference/wire
I'm assuming that there will be 2 new pages for setWireTimeout() and clearWireTimeout()
IMO, the Wire reference documenation is very important as it shows the official API and is a guide for all the 3rd party platform/core developers as well
So any new additions to the Wire API should also include updates the Wire reference page.

BTW, just noticed that end() is not listed on the Wire reference page.... oops...
The Wire end() reference page is where WIRE_HAS_END would be documented.

For these examples, I think it would be useful to at least have a brief comment in the code that mentions that the timeout API functions are not available in all platforms or in older IDE version and perhaps even show an example of using the conditional inside the comment .
OR...
It would be even better to provide links to the specific Wire reference pages for setWireTimeout() and clearWireTimeout() in the comment above their use in the code.
The nice thing about having a link in the comment in the code is that a user can click on it inside the IDE and the IDE will bring up a browser so the user can seamlessly see the additional documentation.

matthijskooijman · 2020-09-26T19:48:03Z

@paynterf, I've taken the liberty to add a commit to your PR that checks for the WIRE_HAS_TIMEOUT macro introduced by #362 (yet to be merged).

I thought about adding the Wire.setWireTimeout() (probably inside WIRE_HAS_TIMEOUT guards) to all examples, but I wonder if that is really useful. These examples are really short and simple and already do not do any error checking at all, so if you would enable timeouts, in these examples, in case of problems they would not lockup, but just end up looping continuously without any output, which might not necessarily be any better.

Adding such a call might be useful to let people realize that there is a timeout feature, but I wonder if it is really worth the extra complexity, especially if we want to enable timeouts by default in the future anyway.

@bperrybap, @greyltc, @cmaglie, @facchinm, any thoughts on this?

CLAassistant · 2021-04-09T15:58:34Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ matthijskooijman
❌ paynterf
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

bperrybap · 2021-04-09T17:02:43Z

@matthijskooijman Good question on if the code should set the timeout.
Ideally, that type of code could benefit from a timeout but I do understand the desire to keep it simple.
Given this code works pretty well "as is" and has for a number of years now, and that the wire library is likely going to enable timeouts by default a some point, then I would lean on keeping this code simple and not doing it.

At most maybe a mention of the timeout capability in the comments?
Or some http links in the comments to the Wire documentation page for information about the Wire API and timeouts
But I have no strong feelings on this.

Added code to demonstrate use of new I2C timeout feature

ae3dd32

matthijskooijman approved these changes Jul 16, 2020

View reviewed changes

matthijskooijman mentioned this pull request Jul 16, 2020

I2C/Wire library: Make Wire library non-blocking #42

Closed

paynterf added 2 commits July 16, 2020 11:00

Move anti-lockup code to beginning of Setup()

8aa1cef

Remove 'clearWireTimeoutFlag()' from setup. It is explicitly cleared …

b9a9ab3

…in 'SetTimeout()'

matthijskooijman approved these changes Jul 16, 2020

View reviewed changes

Add check for WIRE_HAS_TIMEOUT

cf81cfc

per1234 added documentation Improvements or additions to documentation enhancement labels Jan 1, 2022

per1234 mentioned this pull request Jan 3, 2022

Added new Wire functions into libraries/Wire/keywords.txt #355

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added code to demonstrate use of new I2C timeout feature #356

Added code to demonstrate use of new I2C timeout feature #356

paynterf commented Jul 11, 2020

matthijskooijman left a comment

matthijskooijman Jul 16, 2020

paynterf Jul 16, 2020

matthijskooijman Jul 16, 2020

paynterf Jul 16, 2020

matthijskooijman left a comment

bperrybap commented Sep 7, 2020

matthijskooijman commented Sep 17, 2020

bperrybap commented Sep 17, 2020 •

edited

Loading

matthijskooijman commented Sep 26, 2020

CLAassistant commented Apr 9, 2021 •

edited

Loading

bperrybap commented Apr 9, 2021

Added code to demonstrate use of new I2C timeout feature #356

Are you sure you want to change the base?

Added code to demonstrate use of new I2C timeout feature #356

Conversation

paynterf commented Jul 11, 2020

matthijskooijman left a comment

Choose a reason for hiding this comment

matthijskooijman Jul 16, 2020

Choose a reason for hiding this comment

paynterf Jul 16, 2020

Choose a reason for hiding this comment

matthijskooijman Jul 16, 2020

Choose a reason for hiding this comment

paynterf Jul 16, 2020

Choose a reason for hiding this comment

matthijskooijman left a comment

Choose a reason for hiding this comment

bperrybap commented Sep 7, 2020

matthijskooijman commented Sep 17, 2020

bperrybap commented Sep 17, 2020 • edited Loading

matthijskooijman commented Sep 26, 2020

CLAassistant commented Apr 9, 2021 • edited Loading

bperrybap commented Apr 9, 2021

bperrybap commented Sep 17, 2020 •

edited

Loading

CLAassistant commented Apr 9, 2021 •

edited

Loading